Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sqlite: disable memstatus APIs at build time #56541

Merged
merged 1 commit into from
Jan 12, 2025
Merged

sqlite: disable memstatus APIs at build time #56541

merged 1 commit into from
Jan 12, 2025

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 10, 2025

This commit defines SQLITE_DEFAULT_MEMSTATUS=0 for the SQLite build. This setting disables several currently unused C APIs in SQLite, which can yield noticeable performance improvements. This setting is also used by better-sqlite, and is one of the recommended compile-time options in the SQLite docs.

The disabled APIs are used to report statistics about SQLite's memory usage. The drawback to this change is that those APIs could possibly be useful one day. I think the perf tradeoff and prior art make this change worth it.

Refs: https://sqlite.org/compile.html

I tested this code against main, this PR, and better-sqlite by changing the `require()` calls. On my machine, this PR was the fastest (6.4 seconds), followed by better-sqlite (6.7 seconds), followed by main (7.6 seconds). 
'use strict';
const { strictEqual } = require('node:assert');
const { DatabaseSync: Database } = require('node:sqlite');
// const Database = require('better-sqlite3');
const N = 1_000_000;
const db = new Database(':memory:');

db.exec('PRAGMA journal_mode = WAL');
db.exec('CREATE TABLE data(key INTEGER PRIMARY KEY, created_at TEXT DEFAULT CURRENT_TIMESTAMP) STRICT');

const start = performance.now();

for (let i = 0; i < N; ++i) {
  const insert = db.prepare('INSERT INTO data (key) values (?)');
  strictEqual(insert.run(i).lastInsertRowid, i);
  const query = db.prepare('SELECT created_at FROM data WHERE key = ?');
  strictEqual(typeof query.get(i).created_at, 'string');
}

const end = performance.now();
db.close();
console.log('elapsed:', end - start);

@cjihrig
sqlite: disable memstatus APIs at build time
dd72fe3 
This commit defines SQLITE_DEFAULT_MEMSTATUS=0 for the SQLite
build. This setting disables several currently unused C APIs in
SQLite, which can yield noticeable performance improvements.
This setting is also used by better-sqlite, and is one of the
recommended compile-time options in the SQLite docs.

The disabled APIs are used to report statistics about SQLite's
memory usage. The drawback to this change is that those APIs
could possibly be useful one day.

Refs: https://sqlite.org/compile.html
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Jan 10, 2025
@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/64430/

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 10, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 12, 2025
@nodejs-github-bot nodejs-github-bot merged commit c61504b into nodejs:main Jan 12, 2025
75 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c61504b

@cjihrig cjihrig deleted the memstatus branch January 12, 2025 16:37
targos pushed a commit that referenced this pull request Jan 13, 2025
@cjihrig @targos
sqlite: disable memstatus APIs at build time
4f5c9ad 
This commit defines SQLITE_DEFAULT_MEMSTATUS=0 for the SQLite
build. This setting disables several currently unused C APIs in
SQLite, which can yield noticeable performance improvements.
This setting is also used by better-sqlite, and is one of the
recommended compile-time options in the SQLite docs.

The disabled APIs are used to report statistics about SQLite's
memory usage. The drawback to this change is that those APIs
could possibly be useful one day.

Refs: https://sqlite.org/compile.html
PR-URL: #56541
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jan 13, 2025
@cjihrig @Ceres6
sqlite: disable memstatus APIs at build time
315f6bd 
This commit defines SQLITE_DEFAULT_MEMSTATUS=0 for the SQLite
build. This setting disables several currently unused C APIs in
SQLite, which can yield noticeable performance improvements.
This setting is also used by better-sqlite, and is one of the
recommended compile-time options in the SQLite docs.

The disabled APIs are used to report statistics about SQLite's
memory usage. The drawback to this change is that those APIs
could possibly be useful one day.

Refs: https://sqlite.org/compile.html
PR-URL: nodejs#56541
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daeyeon daeyeon daeyeon approved these changes

@targos targos targos approved these changes

@anonrig anonrig anonrig approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dependencies Pull requests that update a dependency file. sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cjihrig @nodejs-github-bot @lpinca @anonrig @targos @daeyeon